-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix chip-tool storage ifdefs to make more sense. #29388
Merged
mergify
merged 1 commit into
project-chip:master
from
bzbarsky-apple:fix-chip-tool-ifdefs
Sep 22, 2023
Merged
Fix chip-tool storage ifdefs to make more sense. #29388
mergify
merged 1 commit into
project-chip:master
from
bzbarsky-apple:fix-chip-tool-ifdefs
Sep 22, 2023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
A few fixes here: 1) CONFIG_USE_LOCAL_STORAGE was being defined to 0 when config_use_local_storage was false, but the tests for it use #ifdef, so setting config_use_local_storage to false did not work right. 2) Commands.cpp was using config options from CHIPDeviceConfig.h without explicitly including it. It happened to be pulled in by KeyValueStoreManager.h, but better to not depend on it. 3) The UseStorageDirectory bits dependency on CHIP_DISABLE_PLATFORM_KVS was set up such that if it was true we ended up with an unused PersistedStorage::KeyValueStoreMgrImpl() return value. This got optimized out in optimized builds, so things worked, but in debug builds we would get link errors. 4) Several Darwin CI jobs that were trying to build the Debug configuraion actually built the Release one, because they did not specify a configuration and that defaults to Release. 5) Remove the unnecessary CHIP_DISABLE_PLATFORM_KVS bits from the Xcode project.
pullapprove
bot
requested review from
andyg-apple,
anush-apple,
arkq,
carol-apple,
cecille,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
electrocucaracha,
gjc13,
harsha-rajendran,
hawk248,
hicklin,
jepenven-silabs,
jmartinez-silabs,
jmeg-sfy,
joonhaengHeo,
jtung-apple,
kkasperczyk-no,
kpschoedel and
ksperling-apple
September 21, 2023 18:21
pullapprove
bot
requested review from
rcasallas-silabs,
robszewczyk,
saurabhst,
selissia,
sharadb-amazon,
tcarmelveilleux,
tecimovic,
tima-q,
tobiasgraf,
turon,
vivien-apple,
woody-apple,
younghak-hwang,
yufengwangca and
yunhanw-google
September 21, 2023 18:21
pullapprove
bot
added
review - pending
and removed
tools
examples
darwin
github
workflows
labels
Sep 21, 2023
krypton36
approved these changes
Sep 21, 2023
PR #29388: Size comparison from 02b8027 to d38491f Decreases (1 build for esp32)
Full report (49 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg)
|
woody-apple
approved these changes
Sep 21, 2023
andy31415
approved these changes
Sep 22, 2023
HunsupJung
pushed a commit
to HunsupJung/connectedhomeip
that referenced
this pull request
Oct 23, 2023
A few fixes here: 1) CONFIG_USE_LOCAL_STORAGE was being defined to 0 when config_use_local_storage was false, but the tests for it use #ifdef, so setting config_use_local_storage to false did not work right. 2) Commands.cpp was using config options from CHIPDeviceConfig.h without explicitly including it. It happened to be pulled in by KeyValueStoreManager.h, but better to not depend on it. 3) The UseStorageDirectory bits dependency on CHIP_DISABLE_PLATFORM_KVS was set up such that if it was true we ended up with an unused PersistedStorage::KeyValueStoreMgrImpl() return value. This got optimized out in optimized builds, so things worked, but in debug builds we would get link errors. 4) Several Darwin CI jobs that were trying to build the Debug configuraion actually built the Release one, because they did not specify a configuration and that defaults to Release. 5) Remove the unnecessary CHIP_DISABLE_PLATFORM_KVS bits from the Xcode project.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A few fixes here:
was false, but the tests for it use #ifdef, so setting
config_use_local_storage to false did not work right.
explicitly including it. It happened to be pulled in by
KeyValueStoreManager.h, but better to not depend on it.
up such that if it was true we ended up with an unused
PersistedStorage::KeyValueStoreMgrImpl() return value. This got optimized
out in optimized builds, so things worked, but in debug builds we would get
link errors.
actually built the Release one, because they did not specify a configuration
and that defaults to Release.